Configure rate limits on VirtualMCPServer PR A by Sanskarzz · Pull Request #5079 · stacklok/toolhive

Sanskarzz · 2026-04-27T20:20:10Z

PR A — shared→global rename + CRD schema for vMCP

Summary

Add the VirtualMCPServer rate-limiting API surface in line with THV-0057.

This change introduces top-level spec.rateLimiting support on VirtualMCPServer and converts that CRD configuration into the generated vMCP runtime config. It also adds global as the preferred field name for the existing MCP server rate-limit configuration while preserving shared as a deprecated compatibility alias.

The CRD validation rejects invalid combinations early, including per-user limits without OIDC incoming auth and any rate limiting without Redis session storage. Generated CRDs and operator docs were updated accordingly.

Runtime vMCP enforcement is intentionally split out of this PR and will be handled in a follow-up PR.

Fixes #4552

Type of change

Test plan

Unit tests (task test)
E2E tests (task test-e2e)
Linting (task lint-fix)
Manual testing (describe below) I will describe full manual testing in follow-up PR

API Compatibility

This PR adds optional VirtualMCPServer.spec.rateLimiting configuration to the v1beta1 API. This is an additive, backward-compatible API change.

Existing VirtualMCPServer resources do not need migration. Cluster admins can opt in by adding spec.rateLimiting and must configure Redis-backed spec.sessionStorage when rate
limiting is enabled. Per-user limits additionally require OIDC incoming auth.

This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Changes

Add VirtualMCPServer.spec.rateLimiting instead of placing rate-limit config under spec.config
Add CEL validation for Redis session storage and OIDC-required per-user limits
Copy spec.rateLimiting into the generated vMCP ConfigMap runtime config
Support global as the primary field name while preserving shared as a compatibility alias
Keep pkg/ratelimit using the CRD RateLimitConfig type directly
Add unit coverage for global serialization and legacy shared Redis key compatibility

github-actions

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.

This review will be automatically dismissed once you add the justification section.

jerm-dro

Hey Sanskar, thanks for putting this together — the end-to-end shape is right: top-level spec.rateLimiting with CEL validation, converter wiring, and the vMCP middleware integration with optimizer-aware tool name resolution. The manual testing scenario in the description is thorough. Requesting changes on two axes before we go further.

1. Drop `pkg/ratelimit/config` — use the CRD type directly

The proxyrunner path already uses *v1beta1.RateLimitConfig directly throughout pkg/ratelimit on main today. vMCP can do the same — just pass the CRD type through. The new package duplicates the CRD types 1:1, still imports metav1.Duration, and adds conversion boilerplate (ToInternal(), hand-written DeepCopy, unused EffectiveGlobal() methods) without solving a real problem.

2. Split the PR

Even after removing pkg/ratelimit/config, this PR bundles several separable changes. At minimum I'd split into:

PR A — shared→global rename + CRD schema for vMCP (pure schema change, no runtime behavior):

Add Global field to RateLimitConfig / ToolRateLimitConfig with shared as deprecated alias
Add spec.rateLimiting to VirtualMCPServerSpec with CEL validation
Converter wiring
Generated CRDs, docs

PR B — vMCP rate-limit middleware wiring (depends on A):

buildRateLimitMiddleware + Redis client lifecycle
Middleware chain refactoring in server.go (this changes execution order — MCP parsing moves before audit — and deserves focused review)
Optimizer call_tool tool-name unwrapping
Unit tests + E2E test

codecov · 2026-04-28T11:42:31Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.57%. Comparing base (8c90184) to head (dc566e1).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5079      +/-   ##
==========================================
+ Coverage   67.53%   67.57%   +0.03%     
==========================================
  Files         601      601              
  Lines       61093    61093              
==========================================
+ Hits        41262    41281      +19     
+ Misses      16714    16696      -18     
+ Partials     3117     3116       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

PR size has been reduced below the XL threshold. Thank you for splitting this up!

github-actions · 2026-04-29T22:08:31Z

✅ PR size has been reduced below the XL threshold. The size review has been dismissed and this PR can now proceed with normal review. Thank you for splitting this up!

jerm-dro · 2026-05-01T23:26:03Z

+	vmcpconfig.Config `yaml:",inline"`
+
+	RateLimiting *mcpv1beta1.RateLimitConfig `yaml:"rateLimiting,omitempty"`
+}


Why introduce this new type? Why not put ratelimiting on vmcpconfig.Config?

As implemented, I don't think the RateLimiting config ever makes it into vmcp's server.go. The CRD field doesn't survive the conversion step, since there's nowhere to write the RateLimiting config to.

You’re right. I had added an operator-side ConfigMap wrapper, but that was incomplete because vMCP loads the file into vmcpconfig.Config, so rateLimiting would be ignored before reaching server.go.
I removed the wrapper and reverted this path to marshal vmcpconfig.Config directly. Since this PR is now scoped to the CRD/schema validation side, I’ll leave the actual runtime propagation/enforcement for PR B, where we can wire the config into vMCP and the middleware properly.

Almost there. You'll want to put RateLimiting *RateLimitConfig on the config type, here, rather than on the CRD type, VirtualMCPServerSpec. The config type is already within the VirtualMCPServerSpec. Putting the RateLimiting field on the config struct means that configuration information will be available to vMCP when it's running. vMCP and most other toolhive services don't read CRDs at runtime, they read config types. If the field is not on the config, then it cannot enable rate limiting.

Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>

Sanskarzz requested review from ChrisJBurns, JAORMX, amirejaz, jerm-dro, jhrozek, lujunsan and yrobla as code owners April 27, 2026 20:20

github-actions Bot previously requested changes Apr 27, 2026

View reviewed changes

github-actions Bot added the size/XL Extra large PR: 1000+ lines changed label Apr 27, 2026

Sanskarzz force-pushed the ratelimitingVMCP branch from dfe941c to d01f3cd Compare April 27, 2026 20:22

github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Apr 27, 2026

jerm-dro requested changes Apr 28, 2026

View reviewed changes